Added use of acceptable_authorization to origin scitokens files #2075
Conversation
-- This will enforce origin capabilities for scitokens -- Adjusted the scitokens origin tests in authorization_test.go to test this functionality -- Added test files to xrootd/resources
| // This will be set to true if PublicReads are enabled as well and is slightly redundant | ||
| // as then a token wouldn't be needed for reads, but doesn't actually change the functionality | ||
| // The default if the acceptable authorization isn't set is "all", so we only care if | ||
| // Reads is Enabled without Writes or vice versa | ||
| if param.Origin_EnableReads.GetBool() && !param.Origin_EnableWrites.GetBool() { | ||
| issuer.AcceptableAuthorization = "read" | ||
| } else if param.Origin_EnableWrites.GetBool() && !param.Origin_EnableReads.GetBool() { | ||
| issuer.AcceptableAuthorization = "write" | ||
| } |
There was a problem hiding this comment.
If I'm interpreting this overall diff correctly, we're setting the issuer's acceptable authorization in the scitokens config per issuer. That's not quite the same as enforcing per-namespace capabilities, especially once #2103 is merged, which allows us to define the prefix<-->issuer relationship as a many-to-many.
Once that PR is in, I think we'll need to change the approach in this section to iterate through each export and determine what the individual issuer(s) for that export should support. That gets us closer to what we want, but still not all the way. Consider a trickier configuration like:
Origin:
Exports:
- FederationPrefix: /foo1
StoragePrefix: /bar1
Capabilities: ["Writes"]
IssuerUrls: ["https://issuer1.com"]
- FederationPrefix: /foo2
StoragePrefix: /bar2
Capabilities: ["Reads"]
IssuerUrls: ["https://issuer1.com"]
In this case, two namespaces define conflicting capabilities but use the same issuer URL. My understanding is this requires us to set all as the acceptable_authorization because otherwise we'd reject tokens for one of the namespaces. But setting all means we'll accept tokens with the wrong scopes for both namespaces. And I don't think we can give two issuers different names for the two different prefixes while having them use the same issuer URL, because XRootD picks up the first URL match and ignores the rest.
Any ideas how we can work around this?
There was a problem hiding this comment.
I think we just have to use the "all", which is what's happening now. There isn't a way to adjust this without a greater change to the xrootd code. This is already not as granular as we'd like (no difference between write/create/modify, etc.) so I think we just need to take what we can get for now.
| @@ -0,0 +1,35 @@ | |||
| # | |||
| # Copyright (C) 2024, Pelican Project, Morgridge Institute for Research | |||
There was a problem hiding this comment.
Since we're officially changing the scitokens config template, can you update the copyright statements?
|
|
||
| err := config.InitServer(ctx, server_structs.OriginType) | ||
| require.NoError(t, err) | ||
| viper.Set("Origin.Port", 8443) |
There was a problem hiding this comment.
I know a lot of this code is going to get hit with merge conflicts so I'm not sure if this comment will be relevant soon, but can you switch any of the viper.Sets you touch to use param.XXX.GetName() for the key (the way it's done in line 786 here)?
| // The default if the acceptable authorization isn't set is "all", so we only care if | ||
| // Reads is Enabled without Writes or vice versa | ||
| if param.Origin_EnableReads.GetBool() && !param.Origin_EnableWrites.GetBool() { | ||
| issuer.AcceptableAuthorization = "read" |
There was a problem hiding this comment.
Can you define "read" and "write" as constants or types?
| // Either one is read and the other is write, in which case we want the default of | ||
| // all, or one is all and the other is read/write, and we'd still want it to be the default | ||
| // of all, which is equivalent to the empty string | ||
| return "" |
There was a problem hiding this comment.
I think I'm more in favor of using "all" in this case instead of relying on the empty string's default behavior. That should make it a bit easier for Fabio to understand the configs we generate without having to find comments in source code.
| [Global] | ||
| audience_json = ["https://origin.example.com:8443"] | ||
|
|
||
| [Issuer Built-in Monitoring] |
There was a problem hiding this comment.
Another note that should help out with these tests is that once #2103 is in, it'll be easier to test this by working with the slice of generated Issuers instead of working with the files on disk. Some of what I did in that PR might break assumptions about preserved ordering of issuers/base paths when everything is written to a file, so you won't be able to do a direct equality comparison. And working directly with the slice of issuers prevents us from having to write new scitokens configs like this every time we need to test a new edge case.
2 participants
-- This will enforce origin capabilities for scitokens -- Adjusted the scitokens origin tests in authorization_test.go to test this functionality
-- Added test files to xrootd/resources